rust-installer/install-template.sh: improve efficiency, step 1. #145809

he32 · 2025-08-24T08:42:01Z

This round replaces repetitive pattern matching in the inner loop of this script using grep (which causes a fork() for each test) with built-in pattern matching in the Bourne shell using the case / esac construct.

This in reference to
#80684
and is a separated-out request from
rust-lang/rust-installer#111

which apparently never got any review.

The forthcoming planned "step 2" change builds on top of this change, and replaces the inner-loops needless uses of sed (which again causes a fork() for each instance) with the suffix removal constructs from the Bourne shell. Since this change touches lots of the same lines this change does, that pull request cannot be submitted before this one is accepted.

Hopefully this first step is less controversial than the latter change.

This round replaces repetitive pattern matching in the inner loop of this script using grep (which causes a fork() for each test) with built-in pattern matching in the bourne shell using the case / esac construct. This in reference to rust-lang#80684 and is a separated-out request from rust-lang/rust-installer#111 which apparently never got any review. The forthcoming planned "step 2" change builds on top of this change, and replaces the inner-loops needless uses of sed (which again causes a fork() for each instance) with the suffix removal constructs from the bourne shell. Since this change touches lots of the same lines this change does, that pull request cannot be submitted before this one is accepted. Hopefully this first step is less controversial than the latter change.

rustbot · 2025-08-24T08:42:05Z

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Kobzol · 2025-08-24T10:01:18Z

Hi, thanks for the PR! Did you try to run any benchmarks (i.e. install a tarball component before/after) the change?

hkBst · 2025-08-24T13:02:48Z

src/tools/rust-installer/install-template.sh

+                    bin/*)
+                        run cp "$_src_dir/$_component/$_file" "$_file_install_path"
+                        run chmod 755 "$_file_install_path"
+                        ;;


This seems like a useful change of behavior, but you did not mention it in your PR summary.

I am a little uncertain what "this" refers to. There was no intention to change the behavior of the script. Note that the test in the previous "if" had an "or", so both tests needs to be covered going forward, and that is what I think the new code does. The case construct can only cover the test for the path name, and cannot test for the executeable-ness of the file.

Ah right, I misread. Still I don't understand why these couldn't be combined like in the original code, perhaps by first testing for "bin"ness and saving the result in a variable, then doing the if-or?

The original code contains 2 copies of "run cp ...; run chmod ...", your code contains 3 copies, but ideally we'd have a single copy...

The two latest commits should have tidied up this issue, there is now just one "run cp ..." command in the pointed-to spot.

he32 · 2025-08-24T14:06:09Z

Hi, thanks for the PR! Did you try to run any benchmarks (i.e. install a tarball component before/after) the change?

Admittedly no. However, this set of modifications came about because this script was absurdly slow when doing the builds / install during my testing of the various rust releases up through the ages, on & for the various different NetBSD targets.

To run a benchmark I would have to figure out how to rig up a test scaffolding for this script, since just doing this as part of a full build will take way too long time.

To me it is sort of obvious that doing a massive number of avoidable fork()s inside the inner loop of any shell script is going to needlessly slow it down, especially when in-shell constructs can achieve the same results. The diff is made by making 1:1 changes which do not change the behavior of the script.

The question can of course come from a couple of different perspectives:

A general desire to quantify the size of the improvement
Whether this change is worth the time it takes to review and get it integrated.
A question of whether this makes any difference at all.

I am hoping the question doesn't come from the third perspective. The second perspective also should not be a valid objection; as the referenced issue describes, it is well known that this script in its current form is just slow.

Mark-Simulacrum · 2025-08-24T16:55:41Z

@bors try

Let's produce some artifacts -- I think the install scripts aren't exercised anywhere(?) in our normal release process, so it'll need some manual testing.

rust-installer/install-template.sh: improve efficiency, step 1.

Mark-Simulacrum · 2025-08-24T16:57:05Z

It's a larger change, but I'm also wondering why this was written as a shell script. AFAICT, this is always bundled into a tarball we produce that contains some kind of binary artifacts. Maybe this could be a Rust program? That would (a) improve our ability to review changes and (b) likely help with performance, at least insofar as we can avoid any sub-shells that aren't needed.

he32 · 2025-08-24T16:58:45Z

OK, I've done some testing.

First off, I install rust 1.88.0 into an empty prefix via

ktrace -i ./install-orig.sh --components=cargo,rust-std-x86_64-unknown-netbsd,rustc --prefix=/var/tmp/t1

to collect system call trace to see how many fork() invocations we have.

kdump | awk '/CALL.*fork/ { n++ } END { print n }'

With the original install.sh script, we get 3910 fork() calls. With the "use case / esac for pattern matching" version we get 2227 fork() calls.

Secondly, doing the install into an already-populated prefix gets us with the original installer the following outputs from "time" in csh:

4.236u 7.596s 0:12.84 92.0%     2546+3121k 0+655io 388pf+0w
4.277u 8.142s 0:13.78 90.0%     2425+2947k 0+649io 379pf+0w
4.212u 7.757s 0:13.37 89.4%     2516+3170k 0+651io 281pf+0w

With the suggested "case / esac for pattern matching" version suggested here, I got

2.823u 5.461s 0:09.31 88.9%     2276+1693k 0+690io 336pf+0w
2.861u 5.468s 0:10.09 82.4%     2265+3026k 0+673io 421pf+0w
2.826u 5.924s 0:09.11 95.9%     2156+2427k 0+673io 5pf+0w

So ... a nearly 30% reduction in wallclock time on this particular host (which is pretty beefy, I predict that the effect will be even more pronounced on slower hosts), and a reduction in the number of fork() invocations of some 43%. And that's before elimination of cut / sed for string suffix / prefix removal, so there is "more to come".

And ... the reason this is particularly noticeable with the "doc" component is most probably that it has a rather larger set of files. The components installed here have relatively few:

$ wc -l cargo/manifest.in rust-std-x86_64-unknown-netbsd/manifest.in rustc/manifest.in
      43 cargo/manifest.in
      28 rust-std-x86_64-unknown-netbsd/manifest.in
      55 rustc/manifest.in
     126 total
$

I don't have a build with the doc component at hand at the moment.

he32 · 2025-08-24T17:13:58Z

It's a larger change, but I'm also wondering why this was written as a shell script. AFAICT, this is always bundled into a tarball we produce that contains some kind of binary artifacts. Maybe this could be a Rust program? That would (a) improve our ability to review changes and (b) likely help with performance, at least insofar as we can avoid any sub-shells that aren't needed.

Yes, that would be a larger change. Why it is the way it is (a shell script), I cannot comment on at the moment. And turning this into a rust program would exceed my current rust abilities, so it would not come from this corner.

However, let me suggest that we first measure the performance improvements we can get with "known fixes" to the existing script to get this from "unbearably slow" to "manageable also on slow hosts" before embarking on that larger rewrite. Expediency has to count for something...

And... It also looks like this avenue has been attempted before, #80684 contains pointers to both similar suggestions (which were not taken), and some which were (the --bulk-dirs for docs). At the end of this it might be worth reviewing those other old suggestions to see which ones are still applicable.

rust-bors · 2025-08-24T19:12:28Z

☀️ Try build successful (CI)
Build commit: 76ab1d0 (76ab1d0bfb8d5b7380aa094bc6be04c5085dae9f, parent: 41a79f1862aa6b81bac674598e275e80e9f09eb9)

he32 · 2025-08-24T19:15:40Z

And ... to preview the suggested next pull request, replacing cut and sed in the inner loop of the script with parameter expansion which does "remove largest suffix pattern" and "remove shortest prefix pattern" modifications reduces the number of fork() invocations further down from the original 3910, improved by the fix in this pull request to 2227, to 1153. Repeating the same test as above gives the following times:

1.401u 3.399s 0:04.64 103.2%    843+1481k 0+655io 20pf+0w
1.450u 3.346s 0:04.58 104.5%    843+971k 0+653io 0pf+0w
1.347u 3.442s 0:04.63 103.2%    845+1218k 0+655io 0pf+0w

So, average wall-clock time of 4.62, which is around 35% of the original timing (reduced by 65%), and the number of fork() invocations is down to around 30% of the original value.

Mark-Simulacrum · 2025-08-28T22:08:03Z

@rust-lang/bootstrap anyone interested in reviewing the changes in this (and future) PRs? I'd personally rather invest the time spent on reviewing complicated shell code into writing a Rust replacement (unless someone finds reasons not to do that).

Kobzol · 2025-08-29T05:51:44Z

I don't know bash much and don't feel confident reviewing this, tbh. By a Rust replacement, you mean writing a Rust crate that will perform the installation process, compiling it for the host target, and then shipping that compiled installed binary in the tarballs, instead of install.sh?

Mark-Simulacrum · 2025-08-29T11:13:37Z

Yes. I guess it might be hard due to being for a particular host target (many of our tarballs don't have a particular host target). But we could either have lots of binaries (like rustup) dispatched via the shell script or universal binaries (though that seems plausibly hard, not sure).

bjorn3 · 2025-08-29T15:19:00Z

It's a larger change, but I'm also wondering why this was written as a shell script. AFAICT, this is always bundled into a tarball we produce that contains some kind of binary artifacts. Maybe this could be a Rust program? That would (a) improve our ability to review changes and (b) likely help with performance, at least insofar as we can avoid any sub-shells that aren't needed.

I think the issue with making it a rust program is that you can install a component for a different target than the host, which doesn't work if the tarball contains an installer for a single target. Especially for the rust-std components it is essential that installing it for a different target works to be able to do cross-compilation.

he32 · 2025-09-09T10:01:14Z

So ... how do we progress this pull request? Yes, I realize that Bourne Shell fluency would be a requirement to OK this change.

he32 · 2025-10-06T07:23:28Z

So... Is there any way to move forward with this pull request / review?

Kobzol · 2025-10-06T07:32:15Z

Asked on Zulip if there's anyone up for reviewing.

hkBst · 2025-10-06T07:46:14Z

My comment from Aug 24 still seems unaddressed...

Shunpoco · 2025-10-06T13:37:49Z

src/tools/rust-installer/install-template.sh

            else
-            run cp "$_src_dir/$_component/$_file" "$_file_install_path"
-            run chmod 644 "$_file_install_path"
+                case "$_file" in


I feel that hiring case statement only for one pattern is too much. How about using a flag?

local _is_bin=0 case "$_file" in etc/*) local _f="$(echo "$_file" | sed 's/^etc\///')" _file_install_path="$CFG_SYSCONFDIR/$_f" ;; bin/*) local _f="$(echo "$_file" | sed 's/^bin\///')" _file_install_path="$CFG_BINDIR/$_f" _is_bin=1 # Turn the flag on ;; # .... esac # ... if [ $_is_bin -eq 1 ] || test -x "$_src_dir/$_component/$_file"; then # use the flag here run cp "$_src_dir/$_component/$_file" "$_file_install_path" run chmod 755 "$_file_install_path" else run cp "$_src_dir/$_component/$_file" "$_file_install_path" run chmod 644 "$_file_install_path"

Something like that should be doable. Stylistically I would instead of 0 and 1 use "true" and "false", since I can then drop the "test" via [. The file-copying could also be moved outside of the test. However...

case / esac are shell built-ins, and are therefore comparatively cheap, as they do not require a new process be fork()ed. Using grep for the pattern matching, as the original code did is what's expensive...

Let's see if I can draft something along the lines suggested.
...now done.

hkBst · 2025-10-06T15:05:21Z

src/tools/rust-installer/install-template.sh

+                    bin/*)
+                        run cp "$_src_dir/$_component/$_file" "$_file_install_path"
+                        run chmod 755 "$_file_install_path"
+                        ;;


The original code contains 2 copies of "run cp ...; run chmod ...", your code contains 3 copies, but ideally we'd have a single copy...

Also factor out common commands to outside of test.

rustbot · 2025-10-18T20:14:23Z

⚠️ Warning ⚠️

This PR is based on an upstream commit that is older than 28 days.

It's recommended to update your branch according to the rustc-dev-guide.

…ts...

hkBst · 2025-10-19T06:47:03Z

src/tools/rust-installer/install-template.sh

+                local _f="$(echo "$_file" | sed 's/^etc\///')"
+                _file_install_path="$CFG_SYSCONFDIR/$_f"


Since this is Bash, we also don't need to use sed to cut off the front of this string, but can use ${_file#*/} to cut off the first part up to and including the first slash.

Yes, I know. This isn't just because this is Bash, it is because it's a POSIX Bourne Shell (which also has that construct). I have a slew of similar changes lined up as "step 2" of the efficiency improvements for this script. So once this pull request gets accepted & applied (I hope that will happen...), a new pull request will follow. Ref. my comments about performance testing above. But since my earlier attempt at getting this all accepted stalled or was ignored, I decided it would be tactically better to try to divide this up in bite-size changes.

I appreciate that, and I see that your (much) earlier attempt did include such changes. I'm not sure the solution is dribbling things out though, especially when related changes get brought up in review. It feels like a waste of my review efforts. Perhaps you could reopen a separate PR with all the changes you had lined up before regarding speeding up this particular script with all my comments to this one addressed? I feel I'm more likely to approve such a PR (not that I have any formal power to do that, but it may count for something...). Given that lack of bash expertise is one of the reasons given for the slow review I'd also recommend commenting all these constructs.

hkBst · 2025-10-19T06:51:17Z

src/tools/rust-installer/install-template.sh

+            share/man/*)
+                local _f="$(echo "$_file" | sed 's/^share\/man\///')"
+                _file_install_path="$CFG_MANDIR/$_f"
+                ;;
+            share/doc/*)
+                # HACK: Try to support overriding --docdir.  Paths with the form
+                # "share/doc/$product/" can be redirected to a single --docdir
+                # path. If the following detects that --docdir has been specified
+                # then it will replace everything preceding the "$product" path
+                # component. The problem here is that the combined rust installer
+                # contains two "products": rust and cargo; so the contents of those
+                # directories will both be dumped into the same directory; and the
+                # contents of those directories are _not_ disjoint. Since this feature
+                # is almost entirely to support 'make install' anyway I don't expect
+                # this problem to be a big deal in practice.
+                if [ "$CFG_DOCDIR" != "<default>" ]
+                then
+                    local _f="$(echo "$_file" | sed 's/^share\/doc\/[^/]*\///')"
+                    _file_install_path="$CFG_DOCDIR/$_f"
+                fi
+                ;;
+            share/*)
+                local _f="$(echo "$_file" | sed 's/^share\///')"
+                _file_install_path="$CFG_DATADIR/$_f"
+                ;;


This could use a nested case statement: after matching "share/" remove it via ${_file#*/}, then match on the remainder. (This would also prevent repeated matching of "share/".)

There is no danger of "multiple matches". The man page for sh(1) on NetBSD says:

The syntax of the case command is case word in [(] pattern) [list] ;& [(] pattern) [list] ;; ... esac The pattern can actually be one or more patterns (see Shell Patterns described later), separated by "|" characters. word is expanded and matched against each pattern in turn, from first to last, with each pattern being expanded just before the match is attempted. When a match is found, pattern comparisons cease, and the associated list, if given, is evaluated. If the list is terminated with ";&" execution then falls through to the following list, if any, without evaluating its pattern, or attempting a match. When a list terminated with ";;" has been executed, or when esac is reached, execution of the case statement is complete. The exit status is that of the last command executed from the last list evaluated, if any, or zero otherwise.

and I am pretty certain that this is identical to what POSIX specifies. So there should be no need to further complicate this construct.

I'm not talking about danger; this is just another optimization.

It is ... unproven that this makes much of a difference performance-wise. Can you benchmark what difference it would make?

If for some reason, you have a preference for this code to remain written in a way in which it quite obviously does more work than necessary, and have benchmarks that show that it matters only negligibly, then I'll grudgingly accept that, but it still wouldn't be a very good reason not to write it the efficient way. The efficient way is also not any more complex, so I can't see any downsides. Can you?

hkBst · 2025-10-19T06:59:54Z

src/tools/rust-installer/install-template.sh

-            then
            run cp "$_src_dir/$_component/$_file" "$_file_install_path"
-            run chmod 755 "$_file_install_path"
+            if $_is_bin || test -x "$_src_dir/$_component/$_file"; then


If we're testing whether files are executable to decide the mod bits to set, then why do we also need to check whether they are in the bin/ directory? Are there perhaps some files in the bin/ directory that are missing the executable bit, and we should instead add those missing bits?

I am just trying to faithfully 1:1 replicate the effect of what the original code does, and it has this test. If this should be changed, I would posit that is a separate change.

I don't know if it should be changed, but since I'm noticing this as part of my review I thought I'd bring it up as something that doesn't seem right.

I cannot vouch for all the install components thrown at this script. I am merely trying to faithfully reproduce the actions the script already did before this suggested change.

rustbot assigned Mark-Simulacrum Aug 24, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 24, 2025

hkBst reviewed Aug 24, 2025

View reviewed changes

rust-bors bot added a commit that referenced this pull request Aug 24, 2025

Auto merge of #145809 - he32:installer-perf-fix-1, r=<try>

76ab1d0

rust-installer/install-template.sh: improve efficiency, step 1.

This comment has been minimized.

Sign in to view

Shunpoco reviewed Oct 6, 2025

View reviewed changes

hkBst suggested changes Oct 6, 2025

View reviewed changes

Avoid an extra case/esac: use a flag instead.

27a0720

Also factor out common commands to outside of test.

This comment has been minimized.

Sign in to view

Evidently tidy doesn't like tabs for indentation, even in shell scrip…

9248435

…ts...

hkBst suggested changes Oct 19, 2025

View reviewed changes

		local _f="$(echo "$_file" \| sed 's/^etc\///')"
		_file_install_path="$CFG_SYSCONFDIR/$_f"

rust-installer/install-template.sh: improve efficiency, step 1. #145809

Are you sure you want to change the base?

rust-installer/install-template.sh: improve efficiency, step 1. #145809

Uh oh!

Conversation

he32 commented Aug 24, 2025

Uh oh!

rustbot commented Aug 24, 2025

Uh oh!

Kobzol commented Aug 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

he32 Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

he32 commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-Simulacrum commented Aug 24, 2025

Uh oh!

This comment has been minimized.

Mark-Simulacrum commented Aug 24, 2025

Uh oh!

he32 commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

he32 commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-bors bot commented Aug 24, 2025

Uh oh!

he32 commented Aug 24, 2025

Uh oh!

Mark-Simulacrum commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol commented Aug 29, 2025

Uh oh!

Mark-Simulacrum commented Aug 29, 2025

Uh oh!

bjorn3 commented Aug 29, 2025

Uh oh!

he32 commented Sep 9, 2025

Uh oh!

he32 commented Oct 6, 2025

Uh oh!

Kobzol commented Oct 6, 2025

Uh oh!

hkBst commented Oct 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

he32 Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Oct 18, 2025

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

he32 Aug 24, 2025 •

edited

Loading

he32 commented Aug 24, 2025 •

edited

Loading

he32 commented Aug 24, 2025 •

edited

Loading

he32 commented Aug 24, 2025 •

edited

Loading

Mark-Simulacrum commented Aug 28, 2025 •

edited

Loading

he32 Oct 18, 2025 •

edited

Loading